Add useNetModule to newHttpRequest#49
Conversation
|
Nice one! |
|
@kinyoklion @keelerm84 Hey, can you take a look at this PR? |
|
@abarker-launchdarkly Alan is doing some overlapping work currently. So I am going to defer to him. |
|
@kinyoklion @abarker-launchdarkly any updates on this one? |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit de42f02. Configure here.
| request.setHeader(key, value); | ||
| } | ||
|
|
||
| request.on('response', onResponse); |
There was a problem hiding this comment.
TLS parameters silently ignored when using net module
Medium Severity
When useNetModule is true, the tlsParams argument is accepted but never applied — the electronNet.request() call doesn't incorporate any TLS configuration. A user who sets both useNetModule: true and tlsParams (e.g. a custom ca for self-signed certificates, as shown in LDClient-tls-test.js) will have their TLS configuration silently dropped, potentially causing connection failures or unintended security behavior with no indication of what went wrong.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit de42f02. Configure here.
There was a problem hiding this comment.
I think if we introduce this toggle, then we also need to add to the docs that using native net will delegate tls concerns to chromium (I think). This may not work for everyone.
I would also like to have a warning logged if there is a custom tlsParam defined and we are trying to use net.
There was a problem hiding this comment.
Thanks for the contribution @okejminja and apologies for the very late response.
I can help with moving this along. Overall, I think this PR looks good, but I am concerned about net dropping custom tls configs... I think that if we clearly document this limitation and have sufficient warnings when we run into a possible breaking scenario, then I am good to approve.
Looking forward, we are working on a new major version for this SDK and I am not sure if we would pull this feature over as is. My thoughts are to provide a way for users to override the SDK platform requestor with their https requestor of choice. Would that kind of design satisfy your use case?
| request.setHeader(key, value); | ||
| } | ||
|
|
||
| request.on('response', onResponse); |
There was a problem hiding this comment.
I think if we introduce this toggle, then we also need to add to the docs that using native net will delegate tls concerns to chromium (I think). This may not work for everyone.
I would also like to have a warning logged if there is a custom tlsParam defined and we are trying to use net.
| * Initialization options for the LaunchDarkly Electron SDK. | ||
| */ | ||
| export interface LDOptions extends LDOptionsBase { | ||
| useNetModule?: boolean; |
There was a problem hiding this comment.
See above, I think we need to document the caveats as well as what exactly this option does.
5 participants
Requirements
Related issues
/
Describe the solution you've provided
useNetModulethat will use Electron's net module instead of requesttypesproperty intsconfig.jsonto prevent @types libraries fromnode_modulesfrom being checked when runningnpm run check-typescriptDescribe alternatives you've considered
/
Additional context
/
Note
Medium Risk
Changes the SDK's underlying HTTP transport in the main process when
useNetModuleis enabled, which could affect request behavior across environments (headers, TLS/proxy handling, abort semantics). Scoped behind an opt-in flag with added tests, so overall impact is moderate.Overview
Adds an opt-in
useNetModuleoption that routes main-process HTTP calls through Electron’snetmodule instead of Node’shttp/httpsinnewHttpRequest(with automatic fallback whennetisn’t available).Plumbs the option through
electronPlatformand test harness wiring, updates TypeScript typings to exposeuseNetModule, and adds a focused Jest test suite to validate initialization, request headers (User-Agent), andtrack()behavior when the new transport is enabled.Updates
tsconfig.jsonto set an emptytypeslist sonpm run check-typescriptdoesn’t implicitly include@types/*packages fromnode_modules.Reviewed by Cursor Bugbot for commit de42f02. Bugbot is set up for automated code reviews on this repo. Configure here.